This repository has been archived by the owner on Nov 4, 2024. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 30
refactor: Modify initial loading state for better control of subs section #356
Merged
julianajlk
merged 8 commits into
master
from
julianajlk/REV-3799/order-history-heading-subs-bug
Dec 19, 2023
Merged
refactor: Modify initial loading state for better control of subs section #356
julianajlk
merged 8 commits into
master
from
julianajlk/REV-3799/order-history-heading-subs-bug
Dec 19, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
julianajlk
force-pushed
the
julianajlk/REV-3799/order-history-heading-subs-bug
branch
2 times, most recently
from
December 15, 2023 03:35
62d91a1
to
440e61a
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #356 +/- ##
==========================================
+ Coverage 61.53% 63.58% +2.04%
==========================================
Files 34 34
Lines 325 335 +10
Branches 69 74 +5
==========================================
+ Hits 200 213 +13
+ Misses 118 115 -3
Partials 7 7 ☔ View full report in Codecov by Sentry. |
julianajlk
force-pushed
the
julianajlk/REV-3799/order-history-heading-subs-bug
branch
4 times, most recently
from
December 15, 2023 17:03
555c5a5
to
aa335aa
Compare
julianajlk
force-pushed
the
julianajlk/REV-3799/order-history-heading-subs-bug
branch
from
December 15, 2023 17:38
0ff5a62
to
fbceaa6
Compare
NawfalAhmed
approved these changes
Dec 18, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
MuhammadMattiullah
approved these changes
Dec 18, 2023
julianajlk
deleted the
julianajlk/REV-3799/order-history-heading-subs-bug
branch
December 19, 2023 15:16
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
REV-3799.
There was a pre-existing bug that became apparent when subscriptions was introduced and sunset, since we would show the subscriptions section on a conditional - loading's initial state is false, and this caused the component to render before re-rendering in the loading state when that becomes true.
When
isB2CSubsEnabled
is true:isLoading
is initiallyfalse
, which makes the page to go intorenderOrdersandSubscriptions()
(fromisLoading ? renderLoading() : renderOrdersandSubscriptions()
), which renders<Subscriptions/>
and<OrderHistory/>
components. Once the fetching of orders and subscriptions happen,isLoading
changes totrue
, and the state change makes the component re-render with or without the subscriptions heading depending on whether the user has subs or not.In this PR I've changed the loading initial state to start with
true
for both order-history and subscriptions.I've also refactored the rendering logic to better accommodate the following scenarios: